Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop phony requirement on README_TEMPLATE_FILE #379

Closed
wants to merge 2 commits into from
Closed

Conversation

osterman
Copy link
Member

@osterman osterman commented Feb 9, 2024

what

  • Drop the .PHONY for README_TEMPLATE_FILE

why

  • This breaks our usage of make readme in lots of other places where we generate READMEs based on local templates that differ from the org template. Here are 2 examples.
  • Worst case this file is cached locally.

references

@osterman osterman requested a review from a team as a code owner February 9, 2024 17:31
@github-actions github-actions bot requested review from aknysh and goruha February 9, 2024 17:32
@osterman osterman requested review from Nuru and removed request for goruha and aknysh February 9, 2024 17:32
@osterman osterman requested a review from a team as a code owner February 9, 2024 17:32
Copy link
Contributor

@Nuru Nuru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The breakage cited is not due to local templates being overwritten, it is due to parsing README.md rather than README.yaml
  2. We have no other mechanism for ensuring that the README template gets updated when changed at the source

I am not saying we do not need to change anything, only that this is not the way to fix anything. I will propose a solution once I better understand the full extent of the problem.

@Nuru Nuru closed this in #380 Feb 9, 2024
@Nuru Nuru deleted the drop-phony branch February 9, 2024 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants